Disable and enable extensions #11184

busykai · 2015-05-30T22:55:26Z

This PR provides a simple mechanism to disable and enable again an extension and complete support of the functionality in the Extension Manager.

Extension loader looks for .disabled file in the extension directory, if it's present the information is included and propagated to the entire extensibility subsystem. Extension Manager can disable and enable extensions using the same mechanism.

Extension can also be disabled using `"disable": true` in the package.json. Details: - Rename getPackageJson to getMetadata. - Check for presence of .disabled file in the extension directory and add it to the metadata. - Do not load extension if it is disabled instead, reject with disabled status. - Add DISABLED extension state. - Extension is still gets on record and is enumerated with all its metadata, but it's not started.

busykai · 2015-05-31T01:26:53Z

CC: @albertinad, you made me do it. now you have to review it. :)

mackenza · 2015-05-31T01:31:06Z

src/nls/root/strings.js

    "MARKED_FOR_REMOVAL"                   : "Marked for removal",
    "UNDO_REMOVE"                          : "Undo",
    "MARKED_FOR_UPDATE"                    : "Marked for update",
    "UNDO_UPDATE"                          : "Undo",
+    "MARKED_FOR_DISABLING"                 : "Marked for disabling",


I am not keen on this phrase. Perhaps Marked as disabled would be more appropriate. It doesn't have to follow the semantics of removal and update.

I don't like it either, so we could probably come with something better. Marked as disabled is not good because it implies a completed action. Marked to be disabled, perhaps?

Well it does. Since it is not disabled until the dialog closes, so is not true that it is "Marked as disabled".

You are thinking that the user knows of the implementation. According to the UI, I hit the disable button and now it is Marked as Disabled. This is further backed by the fact that there is an undo link... meaning I am undoing the action of marking as disabled. The user has no idea (or should they) when the actual file gets written to disk.

@TomMalbran is right. User has to be aware anyways: after extension manager is dismissed, there will be a dialog asking for confirmation and if canceled all extensions marked will be unmarked (and left intact).

So what about Marked to be disabled vs Marked for disabling? With all the context, do we need to change it in the first place?

Neither one is English, so it doesn't matter.

You are focusing too much on the disabled part. The action you are taking in the UI is marking and once you click on disable it is marked.

... but choose whatever you want.

The label appears when the action of marking has been taken, so marked is totally appropriate. OK, I will leave it as is for now.

mackenza · 2015-05-31T01:34:49Z

Good stuff. As I mentioned on Slack, I think the .disabled file is a great, simple way of using convention over configuration to make this work. So I definately appreciate that.

Unless I am missing something, or maybe behaviour is different on Linux (Ubuntu 14.04 x64 here), but I don't see how you can enable a disabled extension. I was expecting an Enable button in my "Installed" list in Extension Manager.

busykai · 2015-05-31T01:37:50Z

Unless I am missing something, or maybe behaviour is different on Linux (Ubuntu 14.04 x64 here), but I don't see how you can enable a disabled extension. I was expecting an Enable button in my "Installed" list in Extension Manager.

This sounds as a bug. Checked, it should be Enable. Somehow it regressed while I was writing tests. :( Will fix.

mackenza · 2015-05-31T01:43:44Z

My disabled extensions show up in the list, but the buttons all say Disable

busykai · 2015-05-31T02:03:19Z

@mackenza, fixed it (slightly blushing).

TomMalbran · 2015-05-31T02:47:45Z

src/extensibility/Package.js

+            ExtensionLoader.loadExtension(FileUtils.getBaseName(path), { baseUrl: path }, "main")
+                .done(result.resolve)
+                .fail(result.reject);
+        });


You forgot to return the promise

Thank you! Fixed.

TomMalbran · 2015-05-31T03:15:52Z

You need to unmark all extensions that were marked to be disabled after this line (https://github.com/adobe/brackets/blob/kai/disable-enable-extensions/src/extensibility/ExtensionManagerDialog.js#L209).

TomMalbran · 2015-05-31T03:16:23Z

src/extensibility/ExtensionManagerDialog.js

-                    })
-                    .fail(function (errorArray) {
-                        dlg.close();
-                        ExtensionManager.cleanupUpdates();


Don't you still need line after if (removeErrors) { ?

No, I don't think so. Updates are done always (in parallel) with this change independently of how it went for removes. The only place it needs to be done is when the entire confirmation is cancelled.

zaggino · 2015-05-31T07:57:49Z

question: will this work for default extensions? if so, can we add .disabled to .gitignore file?

busykai · 2015-05-31T13:33:34Z

@zaggino, it will work on default extensions via manually creating and deleting .disabled file.

@TomMalbran, thanks for the thorough review will fix these.

mackenza · 2015-05-31T17:02:29Z

@zaggino @busykai I went into /src/extensions/default/CodeFolding, did a touch .disabled and reloaded Brackets and, indeed, no code folding.

TomMalbran · 2015-05-31T20:41:49Z

src/extensibility/ExtensionManagerDialog.js

+                removeExtensionsPromise = ExtensionManager.removeMarkedExtensions();
+                removeExtensionsPromise
+                    .fail(function (errorArray) {
+                        removeErrors = errorArray;


I think that the code might look better if you parsed the error array here and created the dialog, instead of doing it in the general fail.

There's no need to create dialogs at this scope. The error condition is processed in the .fail of the consolidated promise (Async.waitForAll).

zaggino · 2015-05-31T22:08:28Z

@busykai great, can you add an .disabled entry to the https://github.com/adobe/brackets/blob/master/.gitignore then?

busykai · 2015-06-01T01:21:34Z

@zaggino, you're right. Will add this file to .gitignore.

See #11184.

It could be used to disable default extensions and must never be committed.

Async.waitForAll would not fail, unless failOnReject argument is true.

busykai · 2015-06-01T01:43:10Z

Did some more testing, focused on error handling. Seems to be fine now.

marcelgerber · 2015-06-01T10:20:16Z

.gitignore

@@ -34,3 +34,6 @@ Thumbs.db

 # Files that can be automatically downloaded that we don't want to ship with our builds
 /src/extensibility/node/node_modules/request/tests/
+
+# .disabled should be ignored, it could be used to disable default extensions
+.disabled


This should be more specific: src/extensions/default/*/.disabled

We may also use this file in unit tests for this feature, and in this case it should be included.

I think it's an overkill. It clearly says default extensions there's nothing to misunderstand.

In installer builds, "src" is "www". Don't you want to handle both? (referring to line above, not this one)

Why would we need a .gitignore in installer builds? It's not like the www is a git repo...

@marcelgerber, actually I misunderstood you, you were saying the rule itself should be more specific and I understood you were referring to the comment text. Will fix this.

rroshan1 · 2015-06-22T07:40:09Z

@larz0 @busykai I was thinking of something like this- Update button if applicable should be on a separate line while the disable and remove buttons should be consistent.. Since mostly there won't be many update buttons, the right side won't seem to be heavy either. What do you guys think?

larz0 · 2015-06-23T00:07:00Z

@rroshan1 that works too! We shouldn't be getting more buttons in that area (fingers crossed) so let's go with that 👍

nethip · 2015-06-23T04:11:59Z

@larz0 Thanks!.

@busykai Please go ahead with the suggested changes to this PR, so that it can be merged. Let us target this for 1.4.

busykai · 2015-06-25T13:59:36Z

@rroshan1, simplicity is the best! :) I should have thought about this layout!..

busykai · 2015-06-25T14:29:04Z

@rroshan1, @larz0 how about this? made the buttons a little bit more spread:

or little less spread:

I like the second better.

abose · 2015-06-25T14:42:16Z

I like the first one better. The second one seems a bit out of proportion to me.

lenovouser · 2015-06-25T14:43:27Z

@busykai I think the first one looks better. The second one seems so cluttered.

busykai · 2015-06-25T14:45:00Z

OK. FWIW, the second one is what close to what we used to have (buttons are close to each other).

Added margin of 3px to any extension action button (more spread buttons).

busykai · 2015-06-25T14:50:23Z

@rroshan1, ready for re-review. did not address separate functions comment though. let me know if you feel it has to be addressed.

mackenza · 2015-06-25T15:41:14Z

@busykai agreed 1st screenshot is much better than 2. Even if 2 is what we have now.

side note: why is remove greyed out on "Proper Indent" extension in both SS?

busykai · 2015-06-25T15:57:00Z

@mackenza, it's in src/extensions/dev (space to develop extensions) -- they are not managed via this dialog (but you still should be able to disable your extension if you screwed up :).

EDIT: I've pushed the first version, so more spacious design with this change.

larz0 · 2015-06-26T00:09:51Z

@busykai great, first screenshot is better but could we shave off 2px to the spacing between Disable and Remove buttons?

busykai · 2015-06-26T14:24:28Z

@larz0, like this?

Remove right margin entirely.

busykai · 2015-06-29T16:36:34Z

@larz0, so I've pushed the changes to the layout corresponding to the last image.

@rroshan1, this is ready for review.

larz0 · 2015-06-30T02:29:49Z

@busykai looks good here 👍

rroshan1 · 2015-06-30T09:06:51Z

Thanks @busykai .. 👍
I will get back soon after doing some testing and review.

busykai · 2015-07-07T20:45:54Z

@rroshan1, now fully compliant to the comments.

rroshan1 · 2015-07-08T08:04:28Z

Thanks @busykai..
I have run unit tests and scenario testings from my side and everything looks good and smooth.
Merging it.

Disable and enable extensions

See adobe#11184.

busykai force-pushed the kai/disable-enable-extensions branch from 3d92fdb to 9784ba2 Compare May 30, 2015 23:46

Support disabling/enabling extensions in UI.

c8af09f

busykai force-pushed the kai/disable-enable-extensions branch from 9784ba2 to c8af09f Compare May 31, 2015 00:01

mackenza reviewed May 31, 2015
View reviewed changes

Fix improper chaining of promises.

04e0ae1

TomMalbran reviewed May 31, 2015
View reviewed changes

busykai added 3 commits May 31, 2015 22:24

Address review comments.

6bd7c64

See #11184.

Ignore .disabled.

e4394af

It could be used to disable default extensions and must never be committed.

Handle error condition correctly.

97755d4

Async.waitForAll would not fail, unless failOnReject argument is true.

nethip assigned prksingh Jun 1, 2015

marcelgerber reviewed Jun 1, 2015
View reviewed changes

Merge branch 'master' into kai/disable-enable-extensions

6cc7685

mackenza mentioned this pull request Jun 15, 2015

Use Ctrl+PageUp/PageDown to traverse files in list order #11223

Merged

abose unassigned prksingh Jun 16, 2015

abose added this to the Release 1.4 milestone Jun 23, 2015

Change layout to show the update button separately

03b1e44

Added margin of 3px to any extension action button (more spread buttons).

busykai force-pushed the kai/disable-enable-extensions branch from 398df58 to 03b1e44 Compare June 25, 2015 14:48

Make .disabled file rule more concise.

d7b6675

Change the extension manager layout.

1eef654

Remove right margin entirely.

Use a common function to handle enabling/disabling

753c84c

rroshan1 added a commit that referenced this pull request Jul 8, 2015

Merge pull request #11184 from adobe/kai/disable-enable-extensions

2501136

Disable and enable extensions

rroshan1 merged commit 2501136 into master Jul 8, 2015

busykai deleted the kai/disable-enable-extensions branch July 8, 2015 14:31

sprintr pushed a commit to sprintr/brackets that referenced this pull request Jul 13, 2015

Address review comments.

b27c8ff

See adobe#11184.

This was referenced Aug 30, 2021

[CLOSED] Disable and enable extensions brackets-archive/bracketsIssues#9778

Open

[CLOSED] Use Ctrl+PageUp/PageDown to traverse files in list order brackets-archive/bracketsIssues#9803

Open

Disable and enable extensions #11184

Disable and enable extensions #11184

Conversation

busykai commented May 30, 2015

busykai commented May 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mackenza commented May 31, 2015

busykai commented May 31, 2015

mackenza commented May 31, 2015

busykai commented May 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomMalbran commented May 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaggino commented May 31, 2015

busykai commented May 31, 2015

mackenza commented May 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaggino commented May 31, 2015

busykai commented Jun 1, 2015

busykai commented Jun 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rroshan1 commented Jun 22, 2015

larz0 commented Jun 23, 2015

nethip commented Jun 23, 2015

busykai commented Jun 25, 2015

busykai commented Jun 25, 2015

abose commented Jun 25, 2015

lenovouser commented Jun 25, 2015

busykai commented Jun 25, 2015

busykai commented Jun 25, 2015

mackenza commented Jun 25, 2015

busykai commented Jun 25, 2015

larz0 commented Jun 26, 2015

busykai commented Jun 26, 2015

busykai commented Jun 29, 2015

larz0 commented Jun 30, 2015

rroshan1 commented Jun 30, 2015

busykai commented Jul 7, 2015

rroshan1 commented Jul 8, 2015